Skip to content

refactor(rpc): split the policy write out of the management PUT for the bucket service - #73

Closed
pyropy wants to merge 75 commits into
srdjan/feat/iam-itests-and-docsfrom
srdjan/feat/iam-bucket-policy-header
Closed

pyropy wants to merge 75 commits into
srdjan/feat/iam-itests-and-docsfrom
srdjan/feat/iam-bucket-policy-header

Conversation

@pyropy

@pyropy pyropy commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Splits the policy write out of the management PUT so the bucket service can use it. /s3/bucket/policy (#89) builds on it. CreateBucket is unchanged: a new bucket has no policy until a PutBucketPolicy writes one.

  • bucketpolicysvc.Service.Write, split from Put
  • the bucket service's policyWrites dependency
  • auth.HeaderValue exported
  • InvalidBucketPolicy receipt failure

Change log

  • 2a05921: removed the x-bucket-policy create header and its rollback, after RFC 30 moved the console's default policy to a PutBucketPolicy after the create.

References

🤖 Generated with Claude Code

@pyropy
pyropy added this pull request to stack #71 September 16, 2026 14:13
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from e035fcf to 7b99f81 Compare September 16, 2026 14:49
@pyropy

pyropy commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-02T09:44:16.193280Z 8414874 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 7b99f81938

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@pyropy
pyropy force-pushed the srdjan/feat/iam-itests-and-docs branch from 02c27db to a7af564 Compare September 17, 2026 13:34
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from 7b99f81 to 729b425 Compare September 17, 2026 13:34
@pyropy
pyropy force-pushed the srdjan/feat/iam-itests-and-docs branch from a7af564 to 029adb1 Compare September 21, 2026 11:47
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from 729b425 to eb735f4 Compare September 21, 2026 11:47
@pyropy
pyropy force-pushed the srdjan/feat/iam-itests-and-docs branch from 029adb1 to 620f0ed Compare September 21, 2026 11:50
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from eb735f4 to 7ff2638 Compare September 21, 2026 11:50
@pyropy
pyropy force-pushed the srdjan/feat/iam-itests-and-docs branch from 620f0ed to 10eee79 Compare September 22, 2026 14:17
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch 2 times, most recently from 13fe304 to b73203a Compare September 22, 2026 17:08
@pyropy
pyropy force-pushed the srdjan/feat/iam-itests-and-docs branch from 10eee79 to 5c58938 Compare September 22, 2026 17:08
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch 2 times, most recently from 4302b89 to 6beeafb Compare September 23, 2026 11:55
@pyropy
pyropy force-pushed the srdjan/feat/iam-itests-and-docs branch from 9998ca6 to 5bad0bc Compare September 23, 2026 11:55
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from 6beeafb to ff05320 Compare September 23, 2026 11:55
@pyropy
pyropy force-pushed the srdjan/feat/iam-itests-and-docs branch from d3f8fc6 to bfa1a84 Compare September 23, 2026 12:39
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from ff05320 to 63011b6 Compare September 23, 2026 12:39
@pyropy
pyropy force-pushed the srdjan/feat/iam-itests-and-docs branch from bfa1a84 to dda42ee Compare September 24, 2026 09:07
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch 2 times, most recently from 639c1f2 to 579d49c Compare September 24, 2026 09:13
@pyropy
pyropy force-pushed the srdjan/feat/iam-itests-and-docs branch 2 times, most recently from 0d1eeaa to 8dc6013 Compare September 24, 2026 11:35
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-header branch from 579d49c to 266e6c7 Compare September 24, 2026 11:35
@pyropy
pyropy removed this pull request from stack #71 September 24, 2026 11:48
@pyropy
pyropy added this pull request to stack #86 September 24, 2026 11:49
@pyropy

pyropy commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

pyropy and others added 28 commits October 5, 2026 17:00
…icy routes' tenantId

The package comment documented a "policy" package that does not exist. The
principal policy read routes pass the tenantId through tenantParam like every
other tenant route.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… key created meanwhile waits for it

Two Postgres tests pin what the joined transaction gives the policy write. A
write cancelled after its rotation ran and its principal lock call returned,
before the document is written, leaves the old document with its own grants:
the rotation's delegation writes rolled back with it, where they used to
commit on their own and leave the stored policy paired with another policy's
grants. A key created for an existing principal while the first policy naming
it is written, parked at the same point, waits on the principal row the write
holds until its commit and is then created from the committed policy, which
closes the gap filed as FIL-1389.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tore's fn renames

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The authorizer gains the principal and policy stores and evaluates a
principal-bound key per request. After the region check it loads the key's
principal, answers ListBuckets with s3:ListAllMyBuckets alone, refuses
CreateBucket and DeleteBucket since no policy grants them, resolves the
bucket within the tenant, and computes the principal's effective set from the
bucket's policy. An empty set reads as an unknown bucket, so a principal never
learns a bucket exists that it cannot open; an action outside a non-empty set
is refused. A copy is the same decision taken twice, on the destination's
policy and on the source's. A service key takes the parent RFC's path
unchanged: the action must be in its own permissions, and the bucket in its
own scope or the scope empty, so BucketNotPermitted stays. The key, principal
and policy rows are read with a share lock, so a request that arrives while a
change is committing waits for it and is answered from the new state. A read
that waits out the store's lock timeout fails as TemporarilyUnavailable, a
named failure the gateway retries.

Both kinds of key sign their own per-request delegations: issuer the key,
audience the gateway, subject the bucket, with the expiry rule unchanged. A
principal-bound key holds, over each bucket its principal can reach, the
tenant's delegations for the commands the policy's actions map to, so the
chain the gateway builds runs bucket to tenant to key to gateway, as it does
for a service key scoped to named buckets. The result carries the effective
set as the key's permissions; for a service key it carries the key's own set.

Authorizer.EffectiveActions exposes the principal-and-policy read for the
lookup that /s3/bucket/info makes for a principal-bound key.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t deadlocking

The write holds the policy row and then locks the principal, as its
delegation rotation does, while the read takes the principal and then the
policy. The read's locks each live in their own short transaction, so it
waits for the write and the write never waits on it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ite race test

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
/s3/bucket/info answers both kinds of key from the key's stored grants over
the bucket: one proof chain per grant, from the bucket root through the tenant
to the key. A principal-bound key's permissions are its principal's effective
actions on the bucket, read through the authorizer share-locked with the
principal and the policy, so Info observes the same settled state as
authorize: a removed principal is an unknown key, a bucket the principal holds
no action on is unknown, and a read that waited out the store's lock timeout
is TemporarilyUnavailable.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Info read the effective actions and then listed the access key's delegations
with nothing tying the two together. A policy write rotates the grants and
commits them before the policy row it belongs to becomes visible, so a write
landing between the two reads paired the old actions with the new grants.
Both s3:GetObject and s3:ListBucket map to /content/retrieve, so narrowing a
policy from the two to ListBucket alone reported GetObject over a chain that
still served it.

EffectiveActions now also returns the ETag of the policy the actions came from,
and Info re-reads it after listing the delegations. A policy that moved
meanwhile returns ErrTemporarilyUnavailable, which this path already returns
when a share-locked read waits out a write, so the caller repeats the call.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… info

On Postgres a policy write commits the rotated grants from its callback
before the policy row is written. A principal-bound Info could read the old
actions and ETag, list the new grants, and reread the ETag without a lock
while the write was still in flight, see the same ETag, and return the old
actions over the new proof chain. The reread is now share-locked, so it
waits for the write to commit and then sees the changed ETag; a wait the
store gives up on is ErrTemporarilyUnavailable.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Info rechecked the policy ETag after listing the key's grants but before
building their proof chains. ProofChain reads by audience, command and
subject, not by the listed grant's CID, so a policy write whose rotation
replaced the grants after the recheck made Info pair the old actions with
the new chain, keyed by the old grant's CID. With s3:GetObject and
s3:ListBucket both mapping to /content/retrieve, that answer could restore
an action the write removed. The reread now follows the chain reads, so it
waits on any write whose grants a read could have seen.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ETag

The ETag Info rereads after building the proof chains is the document's
content, so a policy written and written back between Info's reads left it
where it started while the chains Info built came from the first write's
grants, which the second write revoked. Info now lists the key's grants over
the bucket again after the ETag reread, which waited out any write in
flight, and refuses with ErrTemporarilyUnavailable when the set moved. Test:
a policy narrowed before the first list and restored after the first proof
chain, with the original ETag, is refused rather than served.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Adds six scenarios to TestForge for principal-bound access keys: a policy
that scopes a principal to one bucket of its tenant, a Deny statement that
beats an Allow on the same action, a narrowed policy that reaches the swarf
firehose as the revocation of every delegation the member's key held over
the bucket and leaves the member's next request refused, a removed principal
whose keys' delegations are revoked and whose keys and policy entries go with
it, a presigned GET that follows the policy, and the two 422s on the create
route: a principal-bound key asking for permissions, and one naming a
principal the tenant does not have.

The fixture provisions the tenant, creates a service key holding every
permission to make the buckets and seed the objects, and creates the member's
key through the same route with principalId. The console wrapper gains the
principal and policy calls the management client now has, and its
CreateAccessKey takes the request so one call covers both key kinds. The
revocations are read back through the existing awaitRevocations, by the
access key's DID, one per Forge command the granted actions map to. The
harness gains an ingot binary override; swarf needs nothing, since the keys'
grants are revoked through the /ucan/revoke it already serves.

AGENTS.md gains the new packages, the locking and callback convention, the
two key kinds, and the itest overrides.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The IAM scenarios write, read and delete policies with the AWS SDK on the
service key, as the console does, unconditionally; the preconditions are
covered by unit tests. The service key gains the three policy
permissions.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The locking paragraph claimed a failed publish rolls the write back, which
was true of failures up to the publish and not of a failure after the
rotation had committed on its own. The callback's writes now join the
caller's transaction, and the paragraph says so and names the one window
that remains.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t-policy

A CreateBucket request may carry the new bucket's policy as base64 JSON in
the x-bucket-policy header. The header must be covered by the request
signature and decode as a policy document; a refusal is the
InvalidBucketPolicy failure, the name the management API uses. The document
is written right after the bucket row through the policy service's Write,
the same validation and rotation a management-API policy PUT gets: the
principals it names have their keys issued delegations over the new bucket
inside the write. The bucket is deleted if that write fails, so no bucket
outlives a refused or failed policy.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
When a step after the policy write fails, Create's rollback publishes
revocations for the grants that write issued, then deletes the grants,
the policy and the bucket row. It did the deletes even when the publish
failed, so a proof chain a gateway had already fetched outlived every
record that could revoke it. The rollback now returns after a failed
tenant-issuer load or publish, leaving the bucket for a DeleteBucket
retry, which revokes before it deletes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
policyFromHeader read the header through auth.HeaderValue, which treats
an empty value as absent, so a CreateBucket carrying the header with an
empty value created a bucket with no policy and skipped the signed-header
check. A present but empty or whitespace-only header is now an
InvalidBucketPolicy refusal.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tenant key

Create's rollback loaded the tenant issuer before it knew whether there
was anything to revoke, and returned when the load failed. A policy
write that fails on that same key lookup issues no grants, so the
rollback left the bucket row behind: retries got BucketAlreadyOwnedByYou
and DeleteBucket could not remove the row, needing the key and a Sprue
space that was never provisioned. The rollback now lists the bucket's
tenant-issued delegations first and loads the issuer and publishes only
when there is one; with nothing to revoke it deletes directly. A publish
that fails still keeps the bucket for a DeleteBucket retry.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Create's rollback ran on a context detached from the request with no
deadline, and the Swarf client has no timeout of its own, so a
revocation service that accepted the request and never answered hung
the create forever. The rollback's context now carries the Swarf batch
bound, grant.BatchTimeout, on top of the detached context: a client
disconnect still does not abort it, and a publish that times out fails
and keeps the bucket for a DeleteBucket retry.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The fixture kept the fake Swarf under two fields; the deadline-recording
wrapper embeds it, so one field serves both.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…s deletes safe

A create that failed before storing the bucket's root, whose rollback could
not publish the revocations, kept the bucket row for a DeleteBucket retry, but
Delete first asked Sprue whether the space was empty, which needs the root
proof, so the retry could never succeed and the name stayed taken. Delete now
skips the emptiness check when the bucket holds no root: no space a proof can
reach exists for it.

The rollback's deletes shared the publish's deadline, so a publish returning
near it left them an expired context; they now run under a fresh one. They
also deleted the policy before the bucket row, so a failed row delete left a
live bucket without its policy on the memory backend; they now run inside the
policy store's DeleteByBucket callback as Delete's do, delegations and row
first, the policy last.

Tests: a bucket kept without its root is deleted on retry without a Sprue
call and its name is free again; the deletes' deadline is later than the
publish's; a row the rollback cannot delete keeps its policy.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
RFC 30 (113d4ded) makes CreateBucket plain again: a bucket's policy is set
with PutBucketPolicy, never on the create. Create and its rollback return to
their earlier shape (the rollback deletes the bucket row), and the header
constant, its decoding, the blank-header refusal, the rollback's revoke and
publish, the root check a header-less delete needed, and the header tests
and docs go with it.

What PutBucketPolicy builds on stays: the policy service's Write, the
exported auth.HeaderValue, the bucket service's policyWrites dependency, the
InvalidBucketPolicy failure mapping, and the API module in the RPC wiring
test.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Comment thread pkg/rpc/service/bucket/service.go
@pyropy

pyropy commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Folded into #89 (66834c6).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants